Model Config: DB-driven validator, seed sarvamai/elevenlabs/google#859
Model Config: DB-driven validator, seed sarvamai/elevenlabs/google#859vprashrex wants to merge 12 commits into
Conversation
…ons for STT/TTS providers
📝 WalkthroughWalkthroughAdds two Alembic migrations (indexes and seeded STT/TTS model configs), expands ModelConfig provider/completion types and indexes, removes static model/voice whitelists, implements DB-backed blob model/voice validation, integrates validation into config CRUD and job flows, and updates tests accordingly. ChangesModel Provider Expansion and Database-Driven Validation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ai, and ElevenLabs
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
vprashrex
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/models/llm/request.py (1)
265-282:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a return type annotation to
validate_params.The method at line 265 is missing a return type hint. Since it returns
self(aKaapiCompletionConfiginstance), add the return type annotation.Proposed fix
- def validate_params(self): + def validate_params(self) -> "KaapiCompletionConfig":As per coding guidelines:
**/*.pyrequires type hints on all function parameters and return values; Use Python 3.11+ with type hints throughout the codebase.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/models/llm/request.py` around lines 265 - 282, The method validate_params currently lacks a return type annotation; update its signature to annotate the return as the containing class (KaapiCompletionConfig) — i.e., change def validate_params(self) to def validate_params(self) -> "KaapiCompletionConfig" (use a forward reference string if the class is defined later or add from __future__ import annotations) so that callers know it returns self; keep the implementation unchanged and ensure imports/annotations comply with Python 3.11 typing rules.
🧹 Nitpick comments (4)
backend/app/tests/crud/test_model_config.py (1)
214-214: ⚡ Quick winRemove unused
monkeypatchfixture parameters in these tests.Both functions currently trigger Ruff
ARG001and can be simplified safely.♻️ Proposed fix
-def test_validate_blob_none_provider_skips(monkeypatch: pytest.MonkeyPatch) -> None: +def test_validate_blob_none_provider_skips() -> None: @@ -def test_validate_blob_missing_model_raises(monkeypatch: pytest.MonkeyPatch) -> None: +def test_validate_blob_missing_model_raises() -> None:Also applies to: 220-220
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/tests/crud/test_model_config.py` at line 214, The two tests test_validate_blob_none_provider_skips and the other test at the nearby block currently accept an unused monkeypatch fixture causing Ruff ARG001; remove the unused monkeypatch parameter from their function signatures (e.g., change def test_validate_blob_none_provider_skips(monkeypatch: pytest.MonkeyPatch) -> None: to def test_validate_blob_none_provider_skips() -> None:) and run tests to ensure no remaining references to monkeypatch exist.backend/app/alembic/versions/063_seed_stt_tts_model_configs.py (1)
32-67: ⚡ Quick winAdd return type hints for
upgradeanddowngrade.Please annotate both functions with
-> Nonefor consistency with the project typing rule.As per coding guidelines: "
**/*.py: Always add type hints to all function parameters and return values in Python code".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/alembic/versions/063_seed_stt_tts_model_configs.py` around lines 32 - 67, The upgrade and downgrade functions lack return type annotations; add "-> None" to both function definitions (upgrade and downgrade) so their signatures read with a None return type, keeping the bodies unchanged and conforming to the project's typing rule for Python functions.backend/app/alembic/versions/062_add_pending_job_monitoring_indexes.py (1)
35-56: ⚡ Quick winAdd explicit return type hints to migration functions.
upgrade/downgradeshould be annotated as-> Noneto match repo typing standards.As per coding guidelines: "
**/*.py: Always add type hints to all function parameters and return values in Python code".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/alembic/versions/062_add_pending_job_monitoring_indexes.py` around lines 35 - 56, Annotate the migration entry points with explicit return type hints: change the function signatures of upgrade and downgrade to include -> None (i.e., def upgrade() -> None: and def downgrade() -> None:) so they match the repository typing standards and satisfy the rule to add return type hints for all functions like the ones that call op.get_context().autocommit_block() and op.create_index/op.drop_index.backend/app/crud/model_config.py (1)
66-87: ⚡ Quick winType-annotate
_modality_filtersignature.
stmtand return type are currently implicit. Add explicit types (e.g.,Select-based typing) to satisfy repo-wide typing requirements.As per coding guidelines: "
**/*.py: Always add type hints to all function parameters and return values in Python code".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/crud/model_config.py` around lines 66 - 87, Update the _modality_filter signature to add explicit SQLAlchemy Select typing: change def _modality_filter(stmt, completion_type: CompletionType): to def _modality_filter(stmt: Select, completion_type: CompletionType) -> Select: and add the appropriate import for Select (e.g. from sqlalchemy.sql import Select or from sqlalchemy.sql.selectable import Select) at the top of the module; keep all existing logic using ModelConfig, ARRAY, and sqltypes.String unchanged so the function still returns the filtered Select object.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/alembic/versions/063_seed_stt_tts_model_configs.py`:
- Around line 66-83: The current downgrade() uses op.execute(...) to
unconditionally DELETE rows matching provider/model_name which may remove
pre-existing data; either make downgrade() a no-op or modify both upgrade() and
downgrade(): in upgrade() (the INSERT ... ON CONFLICT ... DO NOTHING block) add
a deterministic marker (e.g., set a seeded_by or migration_tag column or INSERT
and capture RETURNING id into a temp table) so you can identify which rows this
revision actually created, and then change downgrade() to only DELETE rows that
match those inserted records (matching the marker or the captured ids) rather
than deleting by provider/model_name alone.
In `@backend/app/crud/model_config.py`:
- Around line 119-132: The docstring for the validation block is inconsistent
with the code: it says native configs must include completion.params.model but
the function returns early for raw_provider.endswith("-native") (see
blob.completion, raw_provider, completion_type) and thus skips that validation;
fix by making them consistent—either remove the early return and enforce the
model presence for all providers (delete the raw_provider.endswith("-native")
return and validate completion.params.model against model_config) or update the
docstring to explicitly state that native providers
(raw_provider.endswith("-native")) are exempt from the model requirement so
callers know native-provider behavior is allowed to omit
completion.params.model.
In `@backend/app/tests/crud/test_model_config.py`:
- Around line 166-168: Add explicit type hints for the helper functions:
annotate _make_blob parameters (e.g., provider: str, completion_type: str,
params: Mapping[str, Any]) and its return type (e.g., SimpleNamespace) and do
the same for the inner function boom (declare its parameters and return type).
Also rename boom's varargs to underscored names (e.g., _args, _kwargs) to
silence unused-arg lint warnings. Apply the same parameter/return annotation
updates to the other helper on lines ~201-203 as well.
---
Outside diff comments:
In `@backend/app/models/llm/request.py`:
- Around line 265-282: The method validate_params currently lacks a return type
annotation; update its signature to annotate the return as the containing class
(KaapiCompletionConfig) — i.e., change def validate_params(self) to def
validate_params(self) -> "KaapiCompletionConfig" (use a forward reference string
if the class is defined later or add from __future__ import annotations) so that
callers know it returns self; keep the implementation unchanged and ensure
imports/annotations comply with Python 3.11 typing rules.
---
Nitpick comments:
In `@backend/app/alembic/versions/062_add_pending_job_monitoring_indexes.py`:
- Around line 35-56: Annotate the migration entry points with explicit return
type hints: change the function signatures of upgrade and downgrade to include
-> None (i.e., def upgrade() -> None: and def downgrade() -> None:) so they
match the repository typing standards and satisfy the rule to add return type
hints for all functions like the ones that call
op.get_context().autocommit_block() and op.create_index/op.drop_index.
In `@backend/app/alembic/versions/063_seed_stt_tts_model_configs.py`:
- Around line 32-67: The upgrade and downgrade functions lack return type
annotations; add "-> None" to both function definitions (upgrade and downgrade)
so their signatures read with a None return type, keeping the bodies unchanged
and conforming to the project's typing rule for Python functions.
In `@backend/app/crud/model_config.py`:
- Around line 66-87: Update the _modality_filter signature to add explicit
SQLAlchemy Select typing: change def _modality_filter(stmt, completion_type:
CompletionType): to def _modality_filter(stmt: Select, completion_type:
CompletionType) -> Select: and add the appropriate import for Select (e.g. from
sqlalchemy.sql import Select or from sqlalchemy.sql.selectable import Select) at
the top of the module; keep all existing logic using ModelConfig, ARRAY, and
sqltypes.String unchanged so the function still returns the filtered Select
object.
In `@backend/app/tests/crud/test_model_config.py`:
- Line 214: The two tests test_validate_blob_none_provider_skips and the other
test at the nearby block currently accept an unused monkeypatch fixture causing
Ruff ARG001; remove the unused monkeypatch parameter from their function
signatures (e.g., change def test_validate_blob_none_provider_skips(monkeypatch:
pytest.MonkeyPatch) -> None: to def test_validate_blob_none_provider_skips() ->
None:) and run tests to ensure no remaining references to monkeypatch exist.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: adea4ba3-9ad8-449f-9bb1-55caa06b92ef
📒 Files selected for processing (11)
backend/app/alembic/versions/062_add_pending_job_monitoring_indexes.pybackend/app/alembic/versions/063_seed_stt_tts_model_configs.pybackend/app/crud/config/config.pybackend/app/crud/config/version.pybackend/app/crud/model_config.pybackend/app/models/llm/constants.pybackend/app/models/llm/request.pybackend/app/models/model_config.pybackend/app/tests/api/routes/configs/test_version.pybackend/app/tests/crud/test_model_config.pybackend/app/tests/models/llm/test_request.py
💤 Files with no reviewable changes (3)
- backend/app/tests/api/routes/configs/test_version.py
- backend/app/tests/models/llm/test_request.py
- backend/app/models/llm/constants.py
| return session.exec(stmt).first() is not None | ||
|
|
||
|
|
||
| def validate_blob_model_or_raise(session: Session, blob: Any) -> None: |
There was a problem hiding this comment.
inline-blob path on /llm/call and /llm/chain no longer validates model/voice. Before this PR, model/voice checks lived in KaapiCompletionConfig.validate_params, so FastAPI ran them on every request body that contained a ConfigBlob — including ad-hoc inline blobs sent to /llm/call. This PR moves the check into validate_blob_model_or_raise (which needs a DB session) and only wires it into ConfigCrud.create_or_raise and ConfigVersionCrud.create_or_raise. The inline-blob branch in services/llm/jobs.py:525 (config_blob = config.blob) never calls it, so a client can now POST {"provider": "google", "type": "tts", "params": {"model": "gemini-99-ultra", "voice": "Nonexistent"}} and the request will be accepted, a job row created, and the failure surfaces deep in the worker instead of as a 4xx at request time
| assert data["data"]["version"]["config_blob"]["completion"]["type"] == "text" | ||
|
|
||
|
|
||
| def test_create_version_with_kaapi_stt_provider_success( |
There was a problem hiding this comment.
can you add these coverage testcases as well
- test_create_version_cannot_change_type_from_stt_to_tts — start with a google + gemini-2.5-pro STT config, try to bump it to google + gemini-2.5-flash-preview-tts TTS, expect 400 immutability error.
- test_create_version_cannot_change_type_from_tts_to_text — start with a google + gemini-2.5-flash-preview-tts TTS config, try to bump it to openai + gpt-4o text, expect 400 immutability error.
- test_create_version_with_kaapi_stt_provider_success — create a google + gemini-2.5-pro STT config, post a new version that just tweaks instructions/temperature, expect 201 with version == 2 and type == "stt".
- test_create_version_with_kaapi_tts_provider_success — create a google + gemini-2.5-flash-preview-tts (voice Kore) TTS config, post a new version switching to e.g. gemini-2.5-pro-preview-tts with a different seeded voice, expect 201 with version == 2 and type == "tts".
| DEFAULT_TTS_VOICE, | ||
| SUPPORTED_MODELS, | ||
| SUPPORTED_VOICES, | ||
| ) |
There was a problem hiding this comment.
here we need to add check that STT/TTS is not supported for OpenAI and send appropriate error message
There was a problem hiding this comment.
in model config there is already an column for input and output modalities which is set .. that has AUDIO, TEXT AND IMAGE ... from that TTS/STT capabilities can be checked
There was a problem hiding this comment.
{
"success": false,
"data": null,
"error": "Provider 'openai' does not support completion type='tts'.",
"errors": null,
"metadata": null
}
…ng database schema for STT/TTS support
…nd update type hints in test functions
…ity and structure
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/app/alembic/versions/063_seed_stt_tts_model_configs.py (1)
32-32: ⚡ Quick winAdd return annotations to the Alembic hooks.
upgrade()anddowngrade()are new Python functions without return type hints, which breaks the repo-wide typing rule.Proposed fix
-def upgrade(): +def upgrade() -> None: @@ -def downgrade(): +def downgrade() -> None:As per coding guidelines,
**/*.py: Always add type hints to all function parameters and return values in Python code.Also applies to: 132-132
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/alembic/versions/063_seed_stt_tts_model_configs.py` at line 32, The Alembic hook functions upgrade() and downgrade() are missing return type annotations; add explicit return type hints (e.g., def upgrade() -> None: and def downgrade() -> None:) to both functions in this migration module so they conform to the repo-wide typing rule and satisfy static type checks; update the function signatures where upgrade and downgrade are defined (including the duplicate at lines noted) to include the -> None return annotation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/alembic/versions/063_seed_stt_tts_model_configs.py`:
- Around line 138-147: The downgrade currently drops the new enum types and
column but leaves rows with providers introduced in this revision, which will
break older code; update the downgrade() in this migration (the function
containing the op.execute calls) to either delete rows with provider IN
('sarvamai','elevenlabs') from global.model_config before reverting the
column/type changes or explicitly raise a RuntimeError/sa_exc to mark the
revision as irreversible; if choosing deletion, run an op.execute("DELETE FROM
global.model_config WHERE provider IN ('sarvamai','elevenlabs')") prior to
dropping/altering the provider enum and column so the pre-063 app can read the
table.
In `@backend/app/crud/model_config.py`:
- Around line 69-74: The query building up stmt using
select(ModelConfig.model_name).where(... ) returns rows in arbitrary DB order;
add an explicit ordering to the statement (e.g., order by ModelConfig.model_name
ascending) before calling session.exec(stmt).all() so the returned list is
deterministic; update the stmt construction that references
select(ModelConfig.model_name), ModelConfig.provider,
ModelConfig.completion_type and ModelConfig.is_active to include an
order_by(...) clause.
---
Nitpick comments:
In `@backend/app/alembic/versions/063_seed_stt_tts_model_configs.py`:
- Line 32: The Alembic hook functions upgrade() and downgrade() are missing
return type annotations; add explicit return type hints (e.g., def upgrade() ->
None: and def downgrade() -> None:) to both functions in this migration module
so they conform to the repo-wide typing rule and satisfy static type checks;
update the function signatures where upgrade and downgrade are defined
(including the duplicate at lines noted) to include the -> None return
annotation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bd1e5f14-5a26-4005-b9aa-0aee8138b963
📒 Files selected for processing (4)
backend/app/alembic/versions/063_seed_stt_tts_model_configs.pybackend/app/crud/model_config.pybackend/app/models/model_config.pybackend/app/tests/crud/test_model_config.py
| op.execute( | ||
| """ | ||
| ALTER TABLE global.model_config | ||
| DROP COLUMN completion_type, | ||
| ALTER COLUMN provider TYPE varchar USING provider::varchar | ||
| """ | ||
| ) | ||
|
|
||
| op.execute("DROP TYPE IF EXISTS global.completion_type_enum") | ||
| op.execute("DROP TYPE IF EXISTS global.provider_enum") |
There was a problem hiding this comment.
Rollback still leaves post-063 providers behind.
downgrade() removes the enum/column changes, but it keeps the sarvamai and elevenlabs rows introduced by this revision. Pre-063 code only handled openai/google, so a rollback can still leave global.model_config unreadable to the older app layer. Either clean up providers introduced here during downgrade or mark the migration as intentionally irreversible.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/alembic/versions/063_seed_stt_tts_model_configs.py` around lines
138 - 147, The downgrade currently drops the new enum types and column but
leaves rows with providers introduced in this revision, which will break older
code; update the downgrade() in this migration (the function containing the
op.execute calls) to either delete rows with provider IN
('sarvamai','elevenlabs') from global.model_config before reverting the
column/type changes or explicitly raise a RuntimeError/sa_exc to mark the
revision as irreversible; if choosing deletion, run an op.execute("DELETE FROM
global.model_config WHERE provider IN ('sarvamai','elevenlabs')") prior to
dropping/altering the provider enum and column so the pre-063 app can read the
table.
| stmt = select(ModelConfig.model_name).where( | ||
| ModelConfig.provider == provider, | ||
| ModelConfig.completion_type == completion_type, | ||
| ModelConfig.is_active, | ||
| ) | ||
| return list(session.exec(stmt).all()) |
There was a problem hiding this comment.
Order the supported-model list before surfacing it.
This list is returned directly in the 400 detail on Lines 141-144. Without an ORDER BY, PostgreSQL can return models in different orders, which makes the API payload and any assertions against it flaky.
Proposed fix
stmt = select(ModelConfig.model_name).where(
ModelConfig.provider == provider,
ModelConfig.completion_type == completion_type,
ModelConfig.is_active,
)
+ stmt = stmt.order_by(ModelConfig.model_name)
return list(session.exec(stmt).all())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/crud/model_config.py` around lines 69 - 74, The query building up
stmt using select(ModelConfig.model_name).where(... ) returns rows in arbitrary
DB order; add an explicit ordering to the statement (e.g., order by
ModelConfig.model_name ascending) before calling session.exec(stmt).all() so the
returned list is deterministic; update the stmt construction that references
select(ModelConfig.model_name), ModelConfig.provider,
ModelConfig.completion_type and ModelConfig.is_active to include an
order_by(...) clause.
3 participants
Target issue is: #693
Summary
Previously, config/version creation accepted any
completion.params.modelwithout validating it againstmodel_config, allowing unsupported or misspelled models to fail only at inference time. STT/TTS providers and models for Google, Sarvamai, and ElevenLabs were also missing frommodel_config.This update adds model validation during config/version creation, seeds missing STT/TTS provider configs, and centralizes provider/model validation within the CRUD layer.
What changed:
validate_blob_model_or_raiseincrud/model_config.py— validates provider,model, completion type, and TTS voice; wired into all job/config entry points
SUPPORTED_MODELS/SUPPORTED_VOICESdicts removed;model_configis now the single source of truth
completion_typecolumn, 4 indexes (composite + GIN), 10 newSTT/TTS rows seeded with pricing for Google models
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.